restore fields in common table settings by Artuomka · Pull Request #1513 · rocket-admin/rocketadmin

Artuomka · 2026-01-21T14:17:16Z

No description provided.

…r commonTableSettings

…ation - Implement tests to verify that personal table settings take precedence over common settings when both are defined. - Add test cases to ensure that common settings are used when personal settings do not exist. - Include a test to check that common settings are applied when personal list_fields is an empty array. - Update mock factory to ensure compatibility with new settings structure.

…ettings retrieval in use cases

Copilot

Pull request overview

This PR restores fields (list_fields, list_per_page, ordering, ordering_field) to common table settings, allowing these settings to be stored at the table level rather than only in personal settings. The changes ensure that when personal settings don't specify these fields (or specify empty arrays), the common table settings are used as fallback values.

Changes:

Modified buildDAOsTableSettingsDs to properly fallback to common table settings when personal list_fields is empty
Added list_fields, list_per_page, ordering, and ordering_field to common table settings entity, DTOs, and controllers
Updated multiple use cases to use builtDAOsTableSettings instead of mixing personal and common settings directly
Added comprehensive tests to verify the priority order between personal and common table settings

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
shared-code/src/helpers/data-structures-builders/table-settings.ds.builder.ts	Fixed logic to check if personal list_fields is both an array and non-empty before using it
backend/test/mock.factory.ts	Added type assertion to fix type compatibility
backend/test/ava-tests/saas-tests/table-settings-personal-e2e.test.ts	Added 3 new tests for verifying personal settings priority (one skipped)
backend/test/ava-tests/non-saas-tests/non-saas-table-settings-e2e.test.ts	Added 3 new tests with formatting changes throughout file
backend/src/entities/table/use-cases/*.use.case.ts	Updated to use builtDAOsTableSettings and extract permission flags consistently
backend/src/entities/table/application/data-structures/found-table-rows.ds.ts	Added columns_view field to TableSettingsInRowsDS
backend/src/entities/table-settings/common-table-settings/utils/*.ts	Added support for new fields in table settings entity building/serialization
backend/src/entities/table-settings/common-table-settings/table-settings.controller.ts	Added new parameters to createSettings endpoint with formatting changes
backend/src/entities/table-settings/application/data-structures/*.ds.ts	Added list_fields, list_per_page, ordering, ordering_field properties to DTOs

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-21T14:21:39Z

+			can_add: can_add,
+			icon: icon,
+			allow_csv_export: allow_csv_export,
+			allow_csv_import: allow_csv_import,


The updateSettings method's inputData object is missing the newly added fields (list_per_page, list_fields, ordering, ordering_field). These fields should be included in the inputData to match the changes made to createSettings and ensure the update operation can modify these fields.

Suggested change

allow_csv_import: allow_csv_import,

allow_csv_import: allow_csv_import,

list_per_page: list_per_page,

list_fields: list_fields,

ordering: ordering,

ordering_field: ordering_field,

Copilot · 2026-01-21T14:21:39Z

+	@ApiProperty()
+	allow_csv_import: boolean;
+
+	@ApiProperty({ isArray: true, type: 'string' })


The ApiProperty decorator uses a string literal 'string' instead of the proper type. It should use type: String (capital S) to follow NestJS/Swagger conventions consistently with other array properties in this file.

Suggested change

@ApiProperty({ isArray: true, type: 'string' })

@ApiProperty({ isArray: true, type: String })

Copilot · 2026-01-21T14:21:39Z

  @ApiProperty()
  allow_csv_import: boolean;
+
+  @ApiProperty({ required: false})


There's a spacing inconsistency in the ApiProperty decorator. It should be required: false (with a space after the colon) to match the formatting style of the other decorators in this file.

Suggested change

@ApiProperty({ required: false})

@ApiProperty({ required: false })

Copilot · 2026-01-21T14:21:40Z

+			const allowCsvExport = tableSettings?.allow_csv_export ?? true;
+			const allowCsvImport = tableSettings?.allow_csv_import ?? true;
+			const can_delete = tableSettings?.can_delete ?? true;
+			const can_update = tableSettings?.can_update ?? true;
+			const can_add = tableSettings?.can_add ?? true;


Variable naming is inconsistent with TypeScript conventions. The variables allowCsvExport, allowCsvImport, can_delete, can_update, and can_add mix camelCase and snake_case naming styles. For consistency, all these variables should use either camelCase (canDelete, canUpdate, canAdd) or all use snake_case.

Copilot · 2026-01-21T14:21:40Z

+		const allowCsvExport = tableSettings?.allow_csv_export ?? true;
+		const allowCsvImport = tableSettings?.allow_csv_import ?? true;
+		const can_delete = tableSettings?.can_delete ?? true;
+		const can_update = tableSettings?.can_update ?? true;
+		const can_add = tableSettings?.can_add ?? true;


Variable naming is inconsistent with TypeScript conventions. The variables allowCsvExport, allowCsvImport, can_delete, can_update, and can_add mix camelCase and snake_case naming styles. For consistency, all these variables should use either camelCase (canDelete, canUpdate, canAdd) or all use snake_case.

Copilot · 2026-01-21T14:21:41Z

+	async updateSettings(
+		@QueryUuid('connectionId') connectionId: string,
+		@QueryTableName() tableName: string,
+		@Body('search_fields') search_fields: Array<string>,
+		@Body('display_name') display_name: string,
+		@Body('excluded_fields') excluded_fields: Array<string>,
+		@Body('identification_fields') identification_fields: Array<string>,
+		@Body('readonly_fields') readonly_fields: Array<string>,
+		@Body('sensitive_fields') sensitive_fields: Array<string>,
+		@Body('sortable_by') sortable_by: Array<string>,
+		@Body('autocomplete_columns') autocomplete_columns: Array<string>,
+		@Body('customFields') customFields: Array<CustomFieldsEntity>,
+		@Body('identity_column') identity_column: string,
+		@Body('can_delete') can_delete: boolean,
+		@Body('can_update') can_update: boolean,
+		@Body('can_add') can_add: boolean,
+		@Body('icon') icon: string,
+		@Body('allow_csv_export') allow_csv_export: boolean,
+		@Body('allow_csv_import') allow_csv_import: boolean,
+		@UserId() userId: string,
+		@MasterPassword() masterPwd: string,
+	): Promise<FoundTableSettingsDs> {


The updateSettings method does not accept the new parameters (list_per_page, list_fields, ordering, ordering_field) that were added to createSettings. This creates an inconsistency where these fields can be set during creation but cannot be updated. The updateSettings method should accept these same parameters to ensure consistency with createSettings.

Copilot · 2026-01-21T14:21:41Z

+test.skip(`${currentTest} should use personal table settings over common table settings when both exist`, async (t) => {
+	const connectionToTestDB = getTestData(mockFactory).connectionToPostgres;
+	const firstUserToken = (await registerUserAndReturnUserInfo(app)).token;
+	const { testTableName, testTableColumnName, testTableSecondColumnName } = await createTestTable(connectionToTestDB);
+
+	const createConnectionResponse = await request(app.getHttpServer())
+		.post('/connection')
+		.send(connectionToTestDB)
+		.set('Cookie', firstUserToken)
+		.set('Content-Type', 'application/json')
+		.set('Accept', 'application/json');
+	const createConnectionRO = JSON.parse(createConnectionResponse.text);
+	t.is(createConnectionResponse.status, 201);
+
+	// Create common table settings with specific list_fields
+	const createTableSettingsDTO = mockFactory.generateTableSettings(
+		createConnectionRO.id,
+		testTableName,
+		[testTableColumnName],
+		undefined,
+		undefined,
+		undefined,
+		undefined,
+		undefined,
+		undefined,
+	);
+	createTableSettingsDTO.list_fields = ['id', testTableColumnName, testTableSecondColumnName];
+	createTableSettingsDTO.list_per_page = 10;
+	createTableSettingsDTO.ordering = QueryOrderingEnum.ASC;
+	createTableSettingsDTO.ordering_field = 'id';
+
+	const createTableSettingsResponse = await request(app.getHttpServer())
+		.post(`/settings?connectionId=${createConnectionRO.id}&tableName=${testTableName}`)
+		.send(createTableSettingsDTO)
+		.set('Cookie', firstUserToken)
+		.set('Content-Type', 'application/json')
+		.set('Accept', 'application/json');
+	t.is(createTableSettingsResponse.status, 201);
+
+	// Create personal table settings with different values
+	const createPersonalTableSettingsDTO: CreatePersonalTableSettingsDto = {
+		list_fields: ['id', testTableColumnName], // different list_fields (fewer columns)
+		list_per_page: 5, // different list_per_page
+		ordering: QueryOrderingEnum.DESC, // different ordering
+		ordering_field: testTableColumnName, // different ordering_field
+		original_names: true,
+		columns_view: [testTableColumnName, 'id'],
+	};
+
+	const createPersonalTableSettingsResponse = await request(app.getHttpServer())
+		.put(`/settings/personal/${createConnectionRO.id}`)
+		.query({ tableName: testTableName })
+		.send(createPersonalTableSettingsDTO)
+		.set('Cookie', firstUserToken)
+		.set('Content-Type', 'application/json')
+		.set('Accept', 'application/json');
+
+	t.is(createPersonalTableSettingsResponse.status, 200);
+
+	// Get table rows and verify personal settings are applied
+	const getTableRowsResponse = await request(app.getHttpServer())
+		.get(`/table/rows/${createConnectionRO.id}?tableName=${testTableName}`)
+		.set('Cookie', firstUserToken)
+		.set('Content-Type', 'application/json')
+		.set('Accept', 'application/json');
+
+	t.is(getTableRowsResponse.status, 200);
+	const getTableRowsRO = JSON.parse(getTableRowsResponse.text);
+
+	// Verify that personal settings list_per_page is used (5 instead of 10)
+	t.is(getTableRowsRO.pagination.perPage, 5);
+
+	// Verify that personal list_fields are used - rows should only have id and testTableColumnName
+	const rowKeys = Object.keys(getTableRowsRO.rows[0]);
+	t.true(rowKeys.includes('id'));
+	t.true(rowKeys.includes(testTableColumnName));
+	// t.false(rowKeys.includes(testTableSecondColumnName)); // was in common settings but not in personal
+});


The first test in this section is marked as skipped with test.skip. This test should either be enabled if it's now working correctly, or there should be a comment explaining why it remains skipped. Skipped tests in a PR can indicate incomplete work or unresolved issues.

Copilot · 2026-01-21T14:21:42Z

+  @ApiProperty({ required: false})
+  list_per_page?: number;
+
+  @ApiProperty({ isArray: true, type: 'string', required: false })


The ApiProperty decorator uses a string literal 'string' instead of the proper type. It should use type: String (capital S) to follow NestJS/Swagger conventions consistently with other array properties in this codebase.

Copilot · 2026-01-21T14:21:42Z

+	const rowKeys = Object.keys(getTableRowsRO.rows[0]);
+	t.true(rowKeys.includes('id'));
+	t.true(rowKeys.includes(testTableColumnName));
+	// t.false(rowKeys.includes(testTableSecondColumnName)); // was in common settings but not in personal


The assertion checking that testTableSecondColumnName is not included in the row keys is commented out. This assertion is important for validating that personal list_fields take priority over common list_fields. If this assertion is failing, it indicates a bug in the implementation. If it's intentionally commented out, there should be a comment explaining why.

Copilot · 2026-01-21T14:21:42Z

+	async (t) => {
+		const connectionToTestDB = getTestData(mockFactory).connectionToPostgres;
+		const firstUserToken = (await registerUserAndReturnUserInfo(app)).token;
+		const { testTableName, testTableColumnName, testTableSecondColumnName } = await createTestTable(connectionToTestDB);


Unused variable testTableSecondColumnName.

Suggested change

const { testTableName, testTableColumnName, testTableSecondColumnName } = await createTestTable(connectionToTestDB);

const { testTableName, testTableColumnName } = await createTestTable(connectionToTestDB);

Artuomka added 3 commits January 21, 2026 12:10

Update list_fields assignment to prioritize personalTableSettings ove…

36fe5f0

…r commonTableSettings

Enhance table settings handling by adding columns_view and updating s…

4cf5dac

…ettings retrieval in use cases

Copilot AI review requested due to automatic review settings January 21, 2026 14:17

Artuomka enabled auto-merge January 21, 2026 14:17

Copilot started reviewing on behalf of Artuomka January 21, 2026 14:17 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

Skip test for personal table settings priority when both settings exist

3ba33f4

Artuomka merged commit 8764f0b into main Jan 21, 2026
19 checks passed

Artuomka deleted the backend_table_settings_rework branch January 21, 2026 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

restore fields in common table settings#1513

restore fields in common table settings#1513
Artuomka merged 4 commits into
mainfrom
backend_table_settings_rework

Artuomka commented Jan 21, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Copilot AI Jan 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-			allow_csv_import: allow_csv_import,
+			allow_csv_import: allow_csv_import,
+			list_per_page: list_per_page,
+			list_fields: list_fields,
+			ordering: ordering,
+			ordering_field: ordering_field,

	@ApiProperty({ isArray: true, type: 'string' })
	@ApiProperty({ isArray: true, type: String })

	@ApiProperty({ required: false})
	@ApiProperty({ required: false })

	const { testTableName, testTableColumnName, testTableSecondColumnName } = await createTestTable(connectionToTestDB);
	const { testTableName, testTableColumnName } = await createTestTable(connectionToTestDB);

Uh oh!

Conversation

Artuomka commented Jan 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants